Skip to content
This repository was archived by the owner on Nov 21, 2025. It is now read-only.

Conversation

@philrz
Copy link
Contributor

@philrz philrz commented Apr 12, 2024

tl;dr

My recent changes to the reference Zeek NDJSON shaper in brimdata/super#5106 unfortunately broke "perf compare" (see this recent Actions failure). When contemplating the multiple ways to address this, I've concluded that the best way forward is to just bring the test data set current by regenerating it from Zeek v6.2.0.

Details

The root cause of the breakage is a bit complex. The prior shaper was from a time when Zeek's ssl logs included a field called client_cert_chain_fuids, but that field was dropped in more recent Zeek releases. The new shaper still works with the newer Zeek logs if the shaper is configured with _crop_records = false since that assigns an inferred type to this field with its unrecognized name. However, the field often appears as an empty array, and that means Zeek assigns the field an inferred type of <[null]>. Since some of the permutations of the perf-compare.sh script output in Zeek TSV, this creates a problem because this is a type that it refuses to output, presumably since there's no obvious Zeek equivalent type to use.

$ zq -version
Version: v1.15.0-7-g0db1b737

$ echo '{"client_cert_chain_fuids":[]}' | zq -f zeek -
type &{}: type cannot be represented in zeek format

There's at least a few ways I could have worked around this, e.g.:

  1. By appending further Zed logic in the shaper just to take care of this special field (and any other similar ones that might pop up after working around that field)
  2. By dropping Zeek TSV output from the run permutations in perf-compare.sh
  3. Putting back some conditional logic like I had in the old shaper to still recognize the old Zeek log revisions and shape them differently, which could include having coverage for this field
  4. Asking the Zed developers to add support for some kind of "good enough" type mapping (e.g., vector[string], since the majority of arrays in Zeek logs end up being of strings... which I was leaning toward a bit since that reference to &{} in the error message makes it pretty impossible to understand, but I also know our Zeek TSV support has been seen at times as hanging by a thread, and given our other priorities, I respect that)

All of these seemed kind of messy and would just prolong the life of a data set that came from software old enough that it's probably being run almost nowhere at this point. The Zed project's connection to Zeek is much smaller than it once was, but we have enough users that still rely on us for security use cases that we've continued to stay current in other ways, such as the recently-created build-zeek repo that brings Brimcap/Zui current with running Zeek v6.2.0. Meanwhile, the Zed storage formats have thankfully held still for a while, so it seems like a fine time to make this jump forward and get current with this test data as well.

I've eyeballed the differences in the old & new data and nothing particularly shocking stood out. The number of log files increased from 26 to 36 thanks to newer parsers and systems added in recent Zeek releases. The counts of some of the Zeek log types we've always had have varied slightly, but not dramatically. Technically this means that the outputs from perf-compare.sh can't be compared apples-to-apples with the ones from the past, but we use Autoperf now as our solution for catching perf regressions. Really, between perf-compare.sh and the related output-check.sh we also run regularly in CI, this repo has mostly been about catching unexpected changes in the output formats and the occasional bug, and I expect it will continue to serve that purpose once this gets merged and becomes a new baseline.

Once this merges, I'll need to merge a small Zed PR to get the CI to run clean again. Specifically, the cut ts in one of the Zed queries in perf-compare.sh needs to become cut quiet(ts) because I let the loaded_scripts logs become part of the updated data set and they lack a ts field and hence the raw cut ts would normally produce an error("missing") that the Zeek TSV writer would refuse to output. We've already been using this technique in the count() by quiet(id.orig_h) query that's already been part of perf-compare.sh, so it seems fine to extend the approach. When I put up that PR, I'll use the opportunity to create a new baseline set of perf numbers for the https://github.com/brimdata/zed/blob/main/performance/README.md page.

@philrz philrz requested review from mattnibs and nwt April 12, 2024 22:36
@philrz philrz self-assigned this Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants